[SPARK-59405][SQL] XSD Validation improvements - #58698
Draft
holdenk wants to merge 2 commits into
Draft
Conversation
Validators used for rowValidationXSDPath are now created through ValidatorUtil.newValidator, which disables external DTD and schema access and enables secure processing on the validator. A JDK-default validator resolves external DTDs and entities found in record data during validate(), before the record parser, which rejects DTDs, sees the record. Records containing such references now fail validation and are handled per parse mode. StaxXMLRecordReader creates a fresh validator per record instead of reusing one through Validator.reset(), which does not retain configuration applied after creation. A validator whose JAXP implementation cannot disable external access now fails the query uniformly instead of marking records corrupt. Co-authored-by: Cursor <cursoragent@cursor.com> Co-Authored-By: Holden Karau <holden@pigscanfly.ca>
StaxXMLRecordReader now keeps one Validator for the lifetime of the reader and re-applies the record-validation configuration after each Validator.reset(), instead of allocating a fresh Validator per record. Validator.reset() does not retain configuration applied after construction, so the secure-processing settings must be re-applied on every reset; ValidatorUtil.reset factors that out so construction and per-record reset share one code path. Reusing the Validator avoids the per-record allocation cost of Schema.newValidator(). Co-authored-by: Cursor <cursoragent@cursor.com> Co-Authored-By: Holden Karau <holden@pigscanfly.ca>
HyukjinKwon
reviewed
Sep 11, 2026
HyukjinKwon
left a comment
HyukjinKwon
left a comment
Member
There was a problem hiding this comment.
Reviewed the XSD validation changes — no blocking issues. A few things I checked that hold up:
- The validator reuse in
StaxXMLRecordReadercorrectly re-applies the secure-processing config afterreset()(which drops post-construction settings), so reused validators keep the same restrictions as freshly created ones. - The
case e: UnsupportedOperationException => throw eadditions are correctly placed before the genericRuntimeExceptioncatch in bothStaxXmlParserandXmlInferSchema, so a JAXP environment error isn't misclassified as a corrupt record. - Disabling external DTD/schema access on the record validator matches the record parser's DTD handling, which is the stated intent.
One optional follow-up (non-blocking): the new test exercises newValidator, but not the reset()-reuse path in StaxXMLRecordReader. A small test that validates a second record through a reused validator would lock in that the secure-processing config survives reset().
Member
|
cc @sandip-db |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Change validation when using rowValidationXSDPath to match the regular validation flow path and re-use
Why are the changes needed?
Current XSD validation is too slow when using rowValidationXSDPath.
Does this PR introduce any user-facing change?
Maybe slight performance improvement
How was this patch tested?
Existing unit tests + new unit test.
Was this patch authored or co-authored using generative AI tooling?
Claude and Cursor